Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement database delegate abstraction for ScriptUtils #551

Closed
wants to merge 3 commits into from

Conversation

AnkBurov
Copy link
Contributor

@AnkBurov AnkBurov commented Jan 21, 2018

Currently ScriptUtils class is tied up with relational databases. This PR introduces database delegate concept allowing for ScriptUtils to work with any database (Cassandra with CQL as an example). It allows Cassandra container (different PR) to use ScriptUtils without any code duplication.

Blocker for PR #525

@@ -14,16 +14,14 @@
* limitations under the License.
*/

package org.testcontainers.jdbc.ext;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this breaks public API

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ScriptUtils is mainly for internal usage (as it's stated in javadoc). No public testcontainers projects are using this class (I checked :) ). So personally I don't think it's a problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know at least one non-public project where it is used (I checked ;) ), so personally I think it is a problem :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, some non-public, probably personal, project uses internal API of another project and therefore internal API of that project must be sealed in stone. The tail wagging the dog.

Okay, I've kept public API of the Sql-based ScriptUtils - wrapper over new ScriptUtils.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably personal

Your assumption is incorrect. These are some big companies actually ;)

Why do you think ScriptUtils is an internal API btw?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you think ScriptUtils is an internal API btw?

It is said in the javadoc:

  • Generic utility methods for working with SQL scripts. Mainly for internal use
  • within the framework.

/**
* Testcontainers container
*/
protected CONTAINER container;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused variable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does used in children - JdbcDatabaseDelegate and CassandraDatabaseDelegate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

children should store it themselves

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

LOGGER.debug("Could not close JDBC Statement", ex);
}

try (DatabaseDelegate closeableDelegate = databaseDelegate) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

executeDatabaseScript(databaseDelegate, ...);
executeDatabaseScript(databaseDelegate, ...)

will not work because you close the delegate in this method. It should not perform such side effects

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be fair, with current code it won't work either - because java.sql.Connection will be closed after first line.
Okay, I created new boolean property in AbstractDatabaseDelegate (to avoid null based logic). Now first statement execution creates new connection. All subsequent statements will use existing connection. Closing the connection closes the connection and sets isConnectionStarted propery to false, if connection was started in the first place. All subsequent database delegate invokations will use the same logic.

It should not perform such side effects

I disagree with you. Connection to the database should be closed after script invocation. As it is in the current diff or current code in master.

* Get or create new connection to the database
*/
protected CONNECTION getConnection() {
if (connection == null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not thread safe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The environment where database delegates are invoked is single threaded, so from my perspective there is no reason to implement here thread safety. Delegate is used to apply a script. No need to even have multiple threads here.

@bsideup bsideup added this to the next milestone Jan 29, 2018
@AnkBurov
Copy link
Contributor Author

AnkBurov commented Feb 2, 2018

All comments are fixed, as usual. Any response?

@rnorth
Copy link
Member

rnorth commented Feb 3, 2018

Sorry, it's been a hectic week. I'm aiming to do some PR re-review/merging tonight, and this is on my list.

rnorth pushed a commit that referenced this pull request Feb 3, 2018
rnorth pushed a commit that referenced this pull request Feb 3, 2018
Abstracted and changed database init script functionality to support use of SQL-like scripts with non-JDBC connections.

Supports #525
@rnorth
Copy link
Member

rnorth commented Feb 3, 2018

I've rebased, squashed and added a changelog (keeping the commit author as yourself though, @AnkBurov). Will merge now from my branch.

@rnorth
Copy link
Member

rnorth commented Feb 3, 2018

Merged into master. Thanks @AnkBurov !

@rnorth rnorth closed this Feb 3, 2018
@AnkBurov AnkBurov deleted the feature/sqlutils branch February 4, 2018 11:11
@AnkBurov
Copy link
Contributor Author

AnkBurov commented Feb 4, 2018

Thanks @rnorth !

@bsideup bsideup removed this from the next milestone Apr 4, 2018

private JdbcDatabaseContainer container;

public JdbcDatabaseDelegate(JdbcDatabaseContainer container) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a design issue here. JdbcDatabaseContainer is a generic type since 4cb5a1d. But here, it's using the raw type. It should take JdbcDatabaseContainer<?> instead.
The current raw type causes Scala, with its more sound type system, to choke.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants